feat: TargetPortNumber int32 to become TargetPorts []Port #1354

capri-xiyue · 2025-08-11T20:20:37Z

part of #1336

change TargetPortNumber int32 to become TargetPorts []Port, where Port includes only a Number field to start in InferencePool v1.
to avoid break change for now, currently TargetPosts only allows exactly one item

netlify · 2025-08-11T20:20:43Z

✅ Deploy Preview for gateway-api-inference-extension ready!

Name	Link
🔨 Latest commit	`b10039a`
🔍 Latest deploy log	https://app.netlify.com/projects/gateway-api-inference-extension/deploys/689d00ac98960a000895f07a
😎 Deploy Preview	https://deploy-preview-1354--gateway-api-inference-extension.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

k8s-ci-robot · 2025-08-11T20:20:47Z

Hi @capri-xiyue. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

liu-cong · 2025-08-11T20:24:27Z

/ok-to-test

capri-xiyue · 2025-08-11T22:57:06Z

/assign @ahg-g

capri-xiyue · 2025-08-11T23:07:27Z

This is ready for early review. I will add additional conversion logic once #1330 gets merged.

/hold until #1330 gets merged

capri-xiyue · 2025-08-11T23:08:25Z

/assign @danehans @nirrozenbaum for InferencePool spec change.

k8s-ci-robot · 2025-08-11T23:13:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: capri-xiyue
Once this PR has been reviewed and has the lgtm label, please ask for approval from ahg-g. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

robscott

Thanks @capri-xiyue! This LGTM, curious what others like @danehans and @nirrozenbaum think.

api/v1/inferencepool_types.go

pkg/epp/controller/inferencepool_reconciler_test.go

robscott

Thanks @capri-xiyue!

api/v1/inferencepool_types.go

site-src/guides/inferencepool-rollout.md

nirrozenbaum

I went over the PR.

the main comment I have is that I think this change is too tightly coupled with a specific way to do DP, while there are potentially multiple ways to do that.

if vLLM decides to shift from multiple ports to something else, we're going to get stuck with an array of size one.
I'm not a big fan of the approach that is suggested in this PR.

if we decide to pursue this path of multiple target ports on the CRD, this PR overall looks good. I left few comments.

nirrozenbaum · 2025-08-12T18:09:50Z

cmd/epp/runner/runner.go

-		"Default value will be set to InferencePool.Spec.TargetPortNumber if not set.")
+		"Default value will be set to InferencePool.Spec.TargetPorts if not set.")


that's not so clear what is the expected behavior now.
if multiple port would be defined will IGW collect metrics from all ports?
I assume that was the intention - we should make that clear if yes and also make this flag multi port flag.
and also - we should also make that happen in the metrics collection which is obviously not the case atm.

Currently we assume that the length of targetPorts is 1. See

I can add additional info to make it clear currently the length of InferencePool.Spec.TargetPorts is 1

nirrozenbaum · 2025-08-12T18:11:00Z

config/charts/inferencepool/templates/inferencepool.yaml

@@ -7,7 +7,8 @@ metadata:
  labels:
    {{- include "gateway-api-inference-extension.labels" . | nindent 4 }}
 spec:
-  targetPortNumber: {{ .Values.inferencePool.targetPortNumber }}
+  targetPorts:
+    - number: {{ .Values.inferencePool.targetPortNumber }}


this doesn't look native.
if we change to targerPorts -> number then we should do the same change in the values.yaml file of the helm chart.

I used this here as currently we just allow one targetPortNumber in targetPorts.(minLength = == maxLength ==1) I agree we should change it once we switch to multiple target ports.

nirrozenbaum · 2025-08-12T18:16:50Z

pkg/epp/handlers/request.go

+		if len(pool.Spec.TargetPorts) != 1 {
+			return fmt.Errorf("expected 1 target port, got %d", len(pool.Spec.TargetPorts))
+		}
+		reqCtx.TargetEndpoint = pod.Address + ":" + strconv.Itoa(int(pool.Spec.TargetPorts[0].Number))


excellent example to show a port is a property of a pod and not of the pool.. this comes back to the discussion on #1336

cmd/epp/runner/runner.go

capri-xiyue · 2025-08-13T20:59:08Z

hold until #1368 gets merged

# Conflicts: # api/v1/inferencepool_types.go # api/v1/zz_generated.deepcopy.go # client-go/applyconfiguration/api/v1/inferencepoolspec.go # Conflicts: # api/v1/zz_generated.deepcopy.go # client-go/applyconfiguration/api/v1/inferencepoolspec.go

# Conflicts: # pkg/epp/controller/inferencepool_reconciler_test.go

Co-authored-by: Rob Scott <[email protected]>

Co-authored-by: Aslak Knutsen <[email protected]>

k8s-ci-robot · 2025-08-13T21:22:50Z

@capri-xiyue: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-gateway-api-inference-extension-verify-main	`b10039a`	link	true	`/test pull-gateway-api-inference-extension-verify-main`
pull-gateway-api-inference-extension-test-unit-main	`b10039a`	link	true	`/test pull-gateway-api-inference-extension-test-unit-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 11, 2025

k8s-ci-robot requested review from kfswain and robscott August 11, 2025 20:20

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 11, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 11, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 11, 2025

k8s-ci-robot assigned ahg-g Aug 11, 2025

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 11, 2025

robscott reviewed Aug 11, 2025

View reviewed changes

api/v1/inferencepool_types.go Outdated Show resolved Hide resolved

capri-xiyue commented Aug 11, 2025

View reviewed changes

pkg/epp/controller/inferencepool_reconciler_test.go Outdated Show resolved Hide resolved

pkg/epp/controller/inferencepool_reconciler_test.go Outdated Show resolved Hide resolved

robscott mentioned this pull request Aug 11, 2025

InferencePool's pod selector cannot select multiple ports within a pod needed for data parallel attention #1336

Open

robscott reviewed Aug 11, 2025

View reviewed changes

api/v1/inferencepool_types.go Outdated Show resolved Hide resolved

api/v1/inferencepool_types.go Show resolved Hide resolved

capri-xiyue requested a review from robscott August 12, 2025 00:21

aslakknutsen reviewed Aug 12, 2025

View reviewed changes

site-src/guides/inferencepool-rollout.md Outdated Show resolved Hide resolved

capri-xiyue requested a review from aslakknutsen August 12, 2025 16:18

nirrozenbaum reviewed Aug 12, 2025

View reviewed changes

capri-xiyue force-pushed the capri-xiyue/target-port-change branch from 1d6392f to a8c8088 Compare August 12, 2025 19:59

nirrozenbaum reviewed Aug 13, 2025

View reviewed changes

cmd/epp/runner/runner.go Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2025

capri-xiyue force-pushed the capri-xiyue/target-port-change branch from de34d3d to bde16d2 Compare August 13, 2025 17:26

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2025

make v1a2 remove inline

1d01ef5

capri-xiyue added 5 commits August 13, 2025 11:13

fixed build failure

681742a

make pointer usage correct

34a6dbc

fixed conversion

b3b06a6

change conversion

8ecb727

fixed linter

f7dcedb

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2025

capri-xiyue force-pushed the capri-xiyue/target-port-change branch from a1e28ad to 575a9e2 Compare August 13, 2025 20:58

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2025

capri-xiyue and others added 17 commits August 13, 2025 14:07

handle nil

637763f

partially fix ut

847aad5

# Conflicts: # pkg/epp/controller/inferencepool_reconciler_test.go

change inferencepool ut temporarily

b8c7cdd

change comments

8d64d61

changed yaml

2ba17b8

added temporary fix

5659619

Update api/v1/inferencepool_types.go

70fdc09

Co-authored-by: Rob Scott <[email protected]>

fixed ut and e2e to adopt naming change

eba4a12

added listtype tag

d18b7ae

revert back:

77cacaf

updated to include list map

9854265

Update site-src/guides/inferencepool-rollout.md

4e15e91

Co-authored-by: Aslak Knutsen <[email protected]>

rebase

98fe3bf

partially fix conflicts

93610f2

rebase with conversion change

0053cd8

updated generate

b10039a

capri-xiyue force-pushed the capri-xiyue/target-port-change branch from e4d080e to b10039a Compare August 13, 2025 21:16

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 13, 2025

		"Default value will be set to InferencePool.Spec.TargetPortNumber if not set.")
		"Default value will be set to InferencePool.Spec.TargetPorts if not set.")

feat: TargetPortNumber int32 to become TargetPorts []Port #1354

Are you sure you want to change the base?

feat: TargetPortNumber int32 to become TargetPorts []Port #1354

Conversation

capri-xiyue commented Aug 11, 2025

Uh oh!

netlify bot commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for gateway-api-inference-extension ready!

Uh oh!

k8s-ci-robot commented Aug 11, 2025

Uh oh!

liu-cong commented Aug 11, 2025

Uh oh!

capri-xiyue commented Aug 11, 2025

Uh oh!

capri-xiyue commented Aug 11, 2025

Uh oh!

capri-xiyue commented Aug 11, 2025

Uh oh!

k8s-ci-robot commented Aug 11, 2025

Uh oh!

robscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nirrozenbaum left a comment

Choose a reason for hiding this comment

Uh oh!

nirrozenbaum Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

capri-xiyue Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirrozenbaum Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

capri-xiyue Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirrozenbaum Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

capri-xiyue commented Aug 13, 2025

Uh oh!

k8s-ci-robot commented Aug 13, 2025

Uh oh!

Uh oh!

netlify bot commented Aug 11, 2025 •

edited

Loading

capri-xiyue Aug 12, 2025 •

edited

Loading

capri-xiyue Aug 12, 2025 •

edited

Loading